Update GetRangeFromAssertions to handle some basic TYP_LONG scenarios where it FitsIn<int32_t>#128906
Update GetRangeFromAssertions to handle some basic TYP_LONG scenarios where it FitsIn<int32_t>#128906tannergooding wants to merge 2 commits into
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR broadens assertion-based range derivation in the JIT so it can reason about some TYP_LONG value numbers when the resulting values are known to fit in int32, and wires the updated API through rangecheck and assertion propagation to enable additional folding / bounds-check reasoning.
Changes:
- Update
ValueNumStore::IsVNIntegralConstantto coerce constants asint64_t, allowingTYP_LONGconstants that fit to be recognized asint32constants. - Extend
RangeCheck::GetRangeFromAssertions/worker to accept an explicitvar_typesand add limited handling forTYP_LONGscenarios (notablyRSZ/RSHshift cases and other VN ops). - Update assertion propagation and range analysis callsites to pass the expression type and tolerate unknown ranges where
TYP_LONGcan’t be represented as anint32-basedRange.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/valuenum.h | Enables integral-constant extraction from TYP_LONG VNs via int64_t coercion. |
| src/coreclr/jit/rangecheck.h | Updates GetRangeFromAssertions signature and adds Range::IsUnknown() helper. |
| src/coreclr/jit/rangecheck.cpp | Implements the new typed assertion-range logic and extends range computation to consult it in more cases. |
| src/coreclr/jit/assertionprop.cpp | Adapts assertion-prop folding to the new API and to possibly-unknown ranges for wider types. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/jit/rangecheck.cpp:796
- In VNF_Cast handling, when
resultis non-constant (e.g., casting to/from types that Range can’t represent) the code unconditionally propagatescastOpRangeif it’s a constant range. This is unsound for sign-changing casts (e.g.,int -> uint,uint -> long) when the operand range can include negatives: the cast changes negative values to large positives, butcastOpRangewould still contain negatives and exclude the large values.
This can cause incorrect tightening and downstream folding/removal based on a range that doesn’t describe the cast result.
// Now see if we can do better by looking at the cast source.
// if its range is within the castTo range, we can use that (and the cast is basically a no-op).
if (varTypeIsIntegral(arg0Typ))
{
Range castOpRange =
GetRangeFromAssertionsWorker(comp, arg0Typ, arg0VN, assertions, --budget, visited);
if (castOpRange.IsConstantRange())
{
if (!result.IsConstantRange())
{
result = castOpRange;
}
else if ((castOpRange.LowerLimit().GetConstant() >= result.LowerLimit().GetConstant()) &&
(castOpRange.UpperLimit().GetConstant() <= result.UpperLimit().GetConstant()))
{
result = castOpRange;
}
}
64aae0c to
2e4ba16
Compare
… where it FitsIn<int32_t>
2e4ba16 to
f72c56d
Compare
…ll GetRangeFromAssertions
|
/azp run fuzzlyn, runtime-coreclr jitstress, runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Diffs are here. Linux Arm64Linux x64Windows Arm64Windows x64Linux armWindows x86Linux x64Windows arm64Windows x64 |
|
Overall the diffs are teh standard set you'd expect. We have places that change from This lights up for places where we're explicitly using |
| } | ||
| else if ((elementCount == 32) && varTypeIsLong(rangeType)) | ||
| { | ||
| return {SymbolicIntegerValue::Zero, UpperBoundForType(TYP_UINT)}; |
There was a problem hiding this comment.
why not just rangeType = TYP_UINT like above?
| *isKnownNonNegative = true; | ||
| } | ||
| if ((rng.LowerLimit().GetConstant() > 0) || (rng.UpperLimit().GetConstant() < 0)) | ||
| Range rng = RangeCheck::GetRangeFromAssertions(this, tree->TypeGet(), treeVN, assertions); |
There was a problem hiding this comment.
I really don't like the fact we need to pass type. It should be evaluated from VN, shouldn't it?
| if ((rng.LowerLimit().GetConstant() > 0) || (rng.UpperLimit().GetConstant() < 0)) | ||
| Range rng = RangeCheck::GetRangeFromAssertions(this, tree->TypeGet(), treeVN, assertions); | ||
|
|
||
| if (rng.IsConstantRange()) |
There was a problem hiding this comment.
it's unfortunate we broke the contract for GetRangeFromAssertions to always return a constant range. My opinion we shouldn't do it and instead either upgrade Range to TYP_LONG or introduce a new GetRangeFromAssertions for 64-bit ranges
| } | ||
| else | ||
| { | ||
| // TODO: We could return `0, keUnknown` for `elementCount == 32` if the result is TYP_LONG |
There was a problem hiding this comment.
I still don't understand what 0, keUnknown is supposed to mean, keUnknown implies it can be something that overflows making the range invalid
|
|
||
| bool IsUnknown() const | ||
| { | ||
| return lLimit.IsUnknown() && uLimit.IsUnknown(); |
This is an alternative to #128676. It needs confirmation that the diffs/TP is acceptable and may require a few iterations or pulling back prior to it being ready for review.